Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix NullHtmlLocalizer for Core5 #7532

Merged
merged 1 commit into from
Nov 11, 2020
Merged

Fix NullHtmlLocalizer for Core5 #7532

merged 1 commit into from
Nov 11, 2020

Conversation

deanmarcussen
Copy link
Member

Fixes #7530

This just makes the interface for WithCulture which is obsolete on the IHtmlLocalizer interface explicit rather than implicit.

This means you can use a ASP.NET Core 3 nuget pack of OrchardCore in a .Net 5 application safely.

Packed locally and tested, works fine.

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>net5.0</TargetFramework>
    <TieredCompilation>true</TieredCompilation>
    <AspNetCoreHostingModel>InProcess</AspNetCoreHostingModel>
    <PreserveCompilationReferences>true</PreserveCompilationReferences>
  </PropertyGroup>

  <ItemGroup>
    <Folder Include="wwwroot\" />
    <Folder Include="Localization\" />
  </ItemGroup>

  <ItemGroup>
    <PackageReference Include="OrchardCore.Logging.NLog" Version="1.0.0-rc2-mypack" />
    <PackageReference Include="OrchardCore.Application.Cms.Targets" Version="1.0.0-rc2-mypack" />
  </ItemGroup>

</Project>

@sebastienros I know we talked about using some compiler directives, but at this point they're not necessary, unless we at some point do a multi target version.

I'm not sure we should - or if we do, we might want to keep the lang version to C# 8 so we don't have to many compiler directives to worry about.

We might put the compiler directives around this to prevent compiler warnings (I think this will actually still compile fine, but there might be a build analyser warning) for people using the source code, and moving the entire OrchardCore solution to .Net 5, but for a separate pr, if we decide we should support that

hishamco
hishamco reviewed Nov 6, 2020
View reviewed changes
src/OrchardCore/OrchardCore/Localization/NullHtmlLocalizerFactory.cs
@@ -56,7 +56,8 @@ public IEnumerable<LocalizedString> GetAllStrings(bool includeParentCultures)
public LocalizedString GetString(string name, params object[] arguments) =>
NullStringLocalizerFactory.NullLocalizer.Instance.GetString(name, arguments);

IHtmlLocalizer IHtmlLocalizer.WithCulture(CultureInfo culture) => Instance;
[Obsolete("This method will be removed in the upcoming ASP.NET Core major release.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we need while the Localization APIs already warn about this. Second thing I'm not sure how this fix related to the referred issue, @agriffard is already fixed most if not issues in the PR #6999

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pr is not about moving to .Net Core 5 @hishamco - If you read the comments on the pr, it is about enabling usage of OrchardCore nugets in a solution targeting .Net Core 5.

@agriffard pr is about actually moving the target framework to .Net Core 5, which is a different thing.

I put the Obsolete in, although not required, so that when we move to .Net Core 5, we will catch it to remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pr is not about moving to .Net Core 5

Ya, but I'm not sure what's the issue if we didn't add this attribute?

Copy link
Contributor

@Skrypt Skrypt Nov 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's going to warn those using it that they should stop using this method because it could be eventually removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm expected ASP.NET Core localization APIs will going to warn insted

@sebastienros
Copy link
Member

I don't think the C# version matters at all. And we can't use C# 9 if we don't use the dotnet 5 sdk which is not public yet.

@deanmarcussen
Copy link
Member Author

I don't think the C# version matters at all. And we can't use C# 9 if we don't use the dotnet 5 sdk which is not public yet.

A comment about the possible issues around multi targeting only.

This pr is purely about getting the ability to run the current Orchard Core nugets, inside a webapp targeting the dotnet 5 pre release framework

@sebastienros
Copy link
Member

C# version is independent from dotnet version, when you multi target it's with dotnet versions. Once an assembly is built, the language version doesn't matter anymore.

@sebastienros
Copy link
Member

Note that we can remove <TieredCompilation>true</TieredCompilation> since it's a default for many versions now

@deanmarcussen deanmarcussen mentioned this pull request Nov 8, 2020
Merged
@agriffard agriffard merged commit a297d7d into dev Nov 11, 2020
@delete-merged-branch delete-merged-branch bot deleted the deanmarcussen/core5-localizer branch November 11, 2020 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hishamco hishamco hishamco left review comments

@Skrypt Skrypt Skrypt left review comments

@sebastienros sebastienros Awaiting requested review from sebastienros

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for .Net Core 5?
5 participants
@deanmarcussen @sebastienros @Skrypt @hishamco @agriffard